Skip to content

feat: wire up opt-in security flag for Identity instances#451

Closed
VaibhavAcharya wants to merge 2 commits into
masterfrom
vaibhavacharya/identity-security-opt-in
Closed

feat: wire up opt-in security flag for Identity instances#451
VaibhavAcharya wants to merge 2 commits into
masterfrom
vaibhavacharya/identity-security-opt-in

Conversation

@VaibhavAcharya

@VaibhavAcharya VaibhavAcharya commented May 27, 2026

Copy link
Copy Markdown
Contributor

- Summary

First of four PRs splitting the Identity security hardening work (originally this PR's combined diff) into reviewable pieces: this one wires up the opt-in mechanism, then one follow-up per issue adds a behavior gated on it.

Some of the remaining Identity security improvements are breaking changes for existing GoTrue clients, so we cannot turn them on globally. This PR adds a per-instance opt-in flag (Security.Strict) that the stricter behaviors hang off. Existing instances keep their current behavior since the flag defaults off. New instances can be made strict by default through a global flag (GOTRUE_NEW_INSTANCES_SECURE_BY_DEFAULT) once the control plane is ready to populate the per-instance security config.

Scaffolding only, no behavior change on its own:

  • Per-instance SecurityConfiguration with the Strict opt-in flag
  • Global GOTRUE_NEW_INSTANCES_SECURE_BY_DEFAULT that flips Strict for new instances created with a config (config-less instances stay permissive to avoid locking themselves out)
  • security_strict exposed on GET /settings

The gated behaviors land in follow-ups: minimum password length (EX-1871), OAuth redirect URI allowlist (EX-1884), and CORS origin allowlist (EX-1883).

- Test plan

  • go build ./... and go vet ./... pass.
  • go test -run 'TestSecurityConfigurationDefaults|TestNewInstancesSecureByDefault' ./conf/... passes.
  • Settings and instance suite tests (TestSettings_SecurityStrict*, InstanceTestSuite/TestCreate_SecureByDefault*) need MySQL (hack/mysqld.sh); run go test ./api/... against it.

- Description for the changelog

Add a per-instance opt-in flag (Security.Strict) and a global secure-by-default flag for Identity instances, ahead of the stricter security behaviors that build on it.

Link EX-1871
Link EX-1884
Link EX-1883

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bbd8f278-64a3-49a4-91b5-372e82616fe1

📥 Commits

Reviewing files that changed from the base of the PR and between c20c091 and d16a894.

📒 Files selected for processing (4)
  • api/instance.go
  • api/instance_test.go
  • conf/configuration.go
  • example.env
🚧 Files skipped from review as they are similar to previous changes (4)
  • example.env
  • api/instance.go
  • conf/configuration.go
  • api/instance_test.go

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Optional secure-by-default for new instances: when enabled, new instances are created with strict security enabled (even if omitted or explicitly set false); explicit true remains preserved.
    • Settings endpoint now exposes a security_strict boolean.
  • Configuration

    • New environment variable to toggle secure-by-default for instance creation (template updated).

Walkthrough

Adds a SecurityConfiguration type (Strict bool) and a GlobalConfiguration flag NewInstancesSecureByDefault; embeds Security into per-instance Configuration. CreateInstance now, when NewInstancesSecureByDefault is enabled and a BaseConfig is provided, forces BaseConfig.Security.Strict = true before persisting. The Settings API response gains a security_strict field populated from config.Security.Strict. Tests and example.env updated to cover defaults and flag-driven behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.31% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: wire up opt-in security flag for Identity instances' accurately summarizes the main change: implementing an opt-in security flag mechanism for Identity instances.
Description check ✅ Passed The description clearly explains the PR's purpose as scaffolding for an opt-in security mechanism, details the configuration changes, and references follow-up PRs for gated behaviors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch vaibhavacharya/identity-security-opt-in

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


Comment @coderabbitai help to get the list of available commands and usage tips.

@VaibhavAcharya VaibhavAcharya force-pushed the vaibhavacharya/identity-security-opt-in branch from 93b64c1 to 400e8bc Compare May 28, 2026 07:35
@VaibhavAcharya VaibhavAcharya marked this pull request as ready for review May 28, 2026 07:50
@VaibhavAcharya VaibhavAcharya requested a review from a team as a code owner May 28, 2026 07:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
conf/configuration.go (1)

131-133: ⚡ Quick win

Align redirect allowlist comment with actual matcher semantics.

The comment says “exact-match allowlist,” but api/external.go:isAllowedRedirectURI allows path-segment prefix matches (e.g., /auth matching /auth/cb). Please update this wording to avoid security-policy misconfiguration by operators.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@conf/configuration.go` around lines 131 - 133, The comment for
AllowedRedirectURIs is misleading (it says "exact-match allowlist") while the
actual matcher is implemented by isAllowedRedirectURI and permits path-segment
prefix matches (e.g., a configured "/auth" allows "/auth/cb"); update the
comment on AllowedRedirectURIs to accurately describe this semantics—state that
entries are matched by scheme, host (or host-only) and by path prefix matching
on path segments (not strict exact path equality), and note any normalization
rules (trailing slashes, default ports) that isAllowedRedirectURI applies so
operators won't misconfigure the allowlist.
api/password_test.go (1)

23-29: ⚡ Quick win

Add a multibyte-password test case to lock in character-length behavior.

Please add a strict-mode case with non-ASCII characters (e.g., emojis/CJK) so the test suite enforces character-count semantics and prevents byte-length regressions.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/password_test.go` around lines 23 - 29, Add a strict-mode test row to the
test cases slice in api/password_test.go that uses the strict identifier and a
multibyte password value (exactly eight Unicode characters, e.g., eight
CJK/emoji characters) and set the expected result to false so the test asserts
character-count (not byte-count) behavior; place it alongside the existing
strict cases (near the "strict at min" / "strict above min" entries) to lock in
multibyte character-length semantics.
api/instance_test.go (1)

78-160: ⚡ Quick win

Add regression coverage for config-less create under secure-by-default.

Please add a test with NewInstancesSecureByDefault=true and no "config" field in the create payload, then assert the instance remains config-less (no forced strict config). This locks in the anti-lockout behavior documented in the handler.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/instance_test.go` around lines 78 - 160, Add a new test method on
InstanceTestSuite (e.g., TestCreate_SecureByDefaultKeepsConfigLess) that sets
ts.API.config.NewInstancesSecureByDefault = true (restore prev with defer),
creates a POST /instances payload that includes only "uuid" (no "config" key)
using a fresh uuid and operatorToken, calls ts.API.handler.ServeHTTP and asserts
StatusCreated, then load the instance with models.GetInstanceByUUID and assert
that i.BaseConfig is nil (or that Security.Strict is not present) to ensure a
config-less create remains config-less under secure-by-default; follow the same
request/encoding/assert patterns used in TestCreate_SecureByDefaultFlipsEnabled
to match style.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/password.go`:
- Around line 17-21: The password length check in validatePassword currently
uses len(password) (byte count) which miscounts UTF-8 characters; change the
check to use utf8.RuneCountInString(password) from the unicode/utf8 package so
the policy enforces character (rune) length correctly, update imports to include
"unicode/utf8" if missing, and keep the same unprocessableEntityError message
using config.Security.MinPasswordLength.

In `@api/settings_test.go`:
- Around line 71-74: The test currently decodes into resp (type Settings) so a
missing security_strict key silently becomes false; change the test to first
decode w.Body into a map[string]json.RawMessage (or map[string]interface{}) and
assert that the key "security_strict" exists, then decode into Settings (resp)
and assert resp.SecurityStrict is false; reference the existing symbols resp,
Settings, SecurityStrict, w.Body and json.NewDecoder to locate the change.

---

Nitpick comments:
In `@api/instance_test.go`:
- Around line 78-160: Add a new test method on InstanceTestSuite (e.g.,
TestCreate_SecureByDefaultKeepsConfigLess) that sets
ts.API.config.NewInstancesSecureByDefault = true (restore prev with defer),
creates a POST /instances payload that includes only "uuid" (no "config" key)
using a fresh uuid and operatorToken, calls ts.API.handler.ServeHTTP and asserts
StatusCreated, then load the instance with models.GetInstanceByUUID and assert
that i.BaseConfig is nil (or that Security.Strict is not present) to ensure a
config-less create remains config-less under secure-by-default; follow the same
request/encoding/assert patterns used in TestCreate_SecureByDefaultFlipsEnabled
to match style.

In `@api/password_test.go`:
- Around line 23-29: Add a strict-mode test row to the test cases slice in
api/password_test.go that uses the strict identifier and a multibyte password
value (exactly eight Unicode characters, e.g., eight CJK/emoji characters) and
set the expected result to false so the test asserts character-count (not
byte-count) behavior; place it alongside the existing strict cases (near the
"strict at min" / "strict above min" entries) to lock in multibyte
character-length semantics.

In `@conf/configuration.go`:
- Around line 131-133: The comment for AllowedRedirectURIs is misleading (it
says "exact-match allowlist") while the actual matcher is implemented by
isAllowedRedirectURI and permits path-segment prefix matches (e.g., a configured
"/auth" allows "/auth/cb"); update the comment on AllowedRedirectURIs to
accurately describe this semantics—state that entries are matched by scheme,
host (or host-only) and by path prefix matching on path segments (not strict
exact path equality), and note any normalization rules (trailing slashes,
default ports) that isAllowedRedirectURI applies so operators won't misconfigure
the allowlist.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b97bd60f-5c32-4763-9cd4-c8a11bdc9ece

📥 Commits

Reviewing files that changed from the base of the PR and between a21f345 and 400e8bc.

📒 Files selected for processing (20)
  • api/admin.go
  • api/api.go
  • api/api_test.go
  • api/context.go
  • api/external.go
  • api/external_test.go
  • api/instance.go
  • api/instance_test.go
  • api/middleware.go
  • api/password.go
  • api/password_test.go
  • api/settings.go
  • api/settings_test.go
  • api/signup.go
  • api/signup_test.go
  • api/user.go
  • api/verify.go
  • conf/configuration.go
  • conf/configuration_test.go
  • example.env

Comment thread api/password.go Outdated
Comment thread api/settings_test.go Outdated
@VaibhavAcharya VaibhavAcharya force-pushed the vaibhavacharya/identity-security-opt-in branch from 400e8bc to 236ab3b Compare May 28, 2026 08:16
@VaibhavAcharya VaibhavAcharya force-pushed the vaibhavacharya/identity-security-opt-in branch from 236ab3b to fca8b32 Compare June 1, 2026 09:27
@VaibhavAcharya VaibhavAcharya changed the title feat: opt-in security hardening for Identity instances feat: wire up opt-in security flag for Identity instances Jun 1, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@api/instance.go`:
- Around line 77-83: The code treats SecurityConfiguration.Strict (bool) the
same for “unset” and explicit false, so update the model and handling to allow
an explicit opt-out: change SecurityConfiguration.Strict from bool to *bool in
conf/configuration.go, update all usages (notably the POST /instances logic that
references params.BaseConfig.Security.Strict and the NewInstancesSecureByDefault
check) to test for nil (treat nil as unset) and only set true when Strict == nil
and NewInstancesSecureByDefault is true, and add a unit test covering explicit
`"security": {"strict": false}` to ensure callers can opt out; alternatively, if
you prefer doc-only fix, update the wording in conf/configuration.go to clearly
state that NewInstancesSecureByDefault overrides both omitted and explicit false
values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bd968bac-8bba-4d30-9e15-643a0394c92d

📥 Commits

Reviewing files that changed from the base of the PR and between 236ab3b and fca8b32.

📒 Files selected for processing (7)
  • api/instance.go
  • api/instance_test.go
  • api/settings.go
  • api/settings_test.go
  • conf/configuration.go
  • conf/configuration_test.go
  • example.env
🚧 Files skipped from review as they are similar to previous changes (4)
  • example.env
  • api/settings.go
  • api/settings_test.go
  • api/instance_test.go

Comment thread api/instance.go
@VaibhavAcharya VaibhavAcharya force-pushed the vaibhavacharya/identity-security-opt-in branch from c20c091 to d16a894 Compare June 1, 2026 10:14
@VaibhavAcharya VaibhavAcharya deleted the vaibhavacharya/identity-security-opt-in branch June 2, 2026 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant